fix(cloudformation): resolve IAM assume role policy intrinsics - #2630
Conversation
|
🎉 Thanks for your first pull request to Floci! Your CI checks need a maintainer to approve them before they run. That is GitHub's standard gate on first-time contributors, not a problem with your PR — so if the checks look like they are doing nothing, that is why. Once a maintainer approves, CI and the compatibility suite start automatically. Nothing is needed from you in the meantime. While you wait, a couple of things that make review faster:
Come join us in Slack — it is the fastest way to reach maintainers if you get stuck, or want feedback on an approach before investing more time in it. |
|
| Filename | Overview |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/cloudformation/provisioners/IamRoleCfnProvisioner.java | Resolves trust-policy intrinsics before persistence, but object- or array-valued Fn::If branches can collapse to empty strings. |
| src/test/java/io/github/hectorvent/floci/services/cloudformation/provisioners/IamRoleCfnProvisionerTest.java | Adds scalar Fn::Sub regression coverage, with inconsistent class-member indentation and no coverage for non-scalar intrinsic results. |
Reviews (1): Last reviewed commit: "fix(cloudformation): resolve IAM assume ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| : "{\"Version\":\"2012-10-17\",\"Statement\":[]}"; | ||
| String assumeDoc = props == null | ||
| ? null | ||
| : ctx.engine().resolveJsonAttribute(props.path("AssumeRolePolicyDocument")); |
There was a problem hiding this comment.
When an AssumeRolePolicyDocument contains an Fn::If whose selected branch is an object or array, resolveJsonAttribute routes that branch through scalar resolution and replaces it with an empty string, causing the role to store a malformed trust policy.
| @Test | ||
| void assumeRolePolicyIntrinsicsAreResolved() { |
There was a problem hiding this comment.
The new test annotation and method declaration are placed at column one rather than using the class-member indentation established throughout this file, while the adjacent ctx() helper also receives unrelated inconsistent indentation. Aligning these lines with the existing test structure keeps this focused fix readable and avoids formatting churn.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks for this, and welcome. Routing AssumeRolePolicyDocument through resolveJsonAttribute is exactly the right fix, it is the same helper the SNS and Step Functions provisioners use for JSON-valued attributes, and your test pins that the stored doc carries no unresolved intrinsics. The missing-document fallback to the empty policy still works since the helper returns null for a missing node. Two small things, neither blocking: (follow-up, separate PR) greptile's point about Fn::If is real but not yours to fix here: the engine collapses an object-valued Fn::If branch to an empty string, and it does that today for every caller of this helper. Worth an issue against the template engine itself. (follow-up) The new test method sits at column one and the ctx() helper's braces drifted; worth a formatting pass if you push again. No blockers from my side. |
2 participants
Summary
Closes #2538
Resolve
AssumeRolePolicyDocumentthroughCloudFormationTemplateEngine.resolveJsonAttributebefore storing it inAWS::IAM::Role.Previously, the raw template node was serialized directly, causing intrinsics such as
Fn::SubandRefto be stored as unresolved CloudFormation syntax.Added a regression test to verify that the resolved trust policy is passed to
IamService.createRolewithout unresolved intrinsic keys.Type of change
fix:)feat:)feat!:orfix!:)AWS Compatibility
The IAM role
AssumeRolePolicyDocumentwas previously stored with unresolved CloudFormation intrinsic functions such asFn::Sub.The fix resolves JSON-valued attributes using the existing
resolveJsonAttributehelper before passing the policy to the IAM service.Checklist
mvn -Dtest=IamRoleCfnProvisionerTest testpasses locally